Skip to content

Fix 3-DC split-brain: monitor assigns wait_primary on replication stall (issue #997)#1149

Open
dimitri wants to merge 6 commits into
mainfrom
fix/issue-997-replication-stall
Open

Fix 3-DC split-brain: monitor assigns wait_primary on replication stall (issue #997)#1149
dimitri wants to merge 6 commits into
mainfrom
fix/issue-997-replication-stall

Conversation

@dimitri

@dimitri dimitri commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Problem

In a 3-DC topology — primary in dc1, standby in dc2, monitor in dc3 — cutting the dc1↔dc2 link while both nodes remain reachable from the monitor leaves synchronous_standby_names set on the primary. Every COMMIT then hangs indefinitely waiting for an acknowledgement from a standby that can no longer be reached.

Closes #997.

Root cause

The FSM had no rule to transition the primary out of the primary state when synchronous replication was stalled. The monitor could see that the standby was unreachable from the primary (via pg_stat_replication reporting sync_state = 'unknown') but took no action.

Fix

Monitor side (src/monitor/):

  • Add a replication_stall_since column to pgautofailover.node. The monitor sets it to now() the first time a primary reports sync_state = unknown (replication partner unreachable) and clears it when replication recovers.
  • Add a region column to pgautofailover.node (populated from --region on pg_autoctl create postgres). This lets operators label nodes with their DC location for observability.
  • Add a GUC pgautofailover.replication_stall_timeout (default 10 s). When now() - replication_stall_since > timeout, the FSM assigns wait_primary, which clears synchronous_standby_names and unblocks writes.
  • Extension bumped to 2.3 with an upgrade path from 2.2.

Client side (src/bin/pg_autoctl/):

  • --region / -G flag added to pg_autoctl create postgres and wired through keeper_config, monitor_register_node, and nodespec.
  • pg_autoctl watch updated to display the region column.

Test (tests/tap/specs/replication_stall_3dc.pgaf):

New end-to-end regression test using the pgaftest framework. The test models the 3-DC scenario in a single Docker Compose stack: the monitor is always reachable from both nodes, but docker network disconnect severs the dc1↔dc2 replication link. Six steps verify:

  1. Sync replication is active (synchronous_standby_names = 'ANY 1 (…)')
  2. The dc1↔dc2 link is cut
  3. The monitor assigns wait_primary within ~20 s (well within the 60 s timeout)
  4. synchronous_standby_names is cleared; writes no longer hang
  5. Link restored; standby re-joins
  6. Row written during the stall has replicated

Verified locally, 6/6 steps passing against PG17.

Watch the fix work, interactively

Bring the exact topology up and drive it step by step (pulls/builds images on first run, ~30s):

pgaftest tmux tests/tap/specs/replication_stall_3dc.pgaf

This opens a 3-pane tmux session: docker compose logs -f (top), pg_autoctl watch — the live FSM state table (middle), an interactive shell (bottom). From the shell:

# See the six steps and where you are
pgaftest show steps

# Preview the DSL commands the next step will run before running it
pgaftest show step

# Confirm sync replication is active: synchronous_standby_names = 'ANY 1 (...)'
pgaftest step

# Sever the dc1<->dc2 link -- watch the middle pane: node1 stays "primary"
# reporting an empty sync_state
pgaftest step
pgaftest show state

# Within ~10-20s (replication_stall_timeout), node1 flips to wait_primary
# in the live watch pane -- this step just asserts what you can already see
pgaftest step

# Confirm writes actually unblocked: synchronous_standby_names is now empty
pgaftest step
pgaftest sql node1 { SHOW synchronous_standby_names; }

# Restore the link; node2 rejoins as secondary, node1 confirms primary
pgaftest step

# Confirm replication caught back up, including the row written during the stall
pgaftest step

# Tear down
pgaftest cluster down

Each pgaftest step (no argument) auto-advances to the next pending step in the spec's sequence; pgaftest show steps at any point lists all six with * marking which one runs next.

To see the bug rather than the fix: the spec file itself doesn't exist before this PR, but the FSM rule it exercises is a single, self-contained if block in ProceedGroupStateFromContext() (src/monitor/group_state_machine.c). Comment that block out, rebuild the monitor image, and rerun the spec — step 3 times out instead of passing (node1 stays at primary with synchronous_standby_names still set), and step 4's write hangs forever (interrupt it and pgaftest cluster down to clean up).

Reusing this for future bug reports

The spec above didn't start as a regression test — it started as the smallest .pgaf file that reliably reproduced the reported bug. That's a generally useful pattern for triaging issues in this repo, not just for this one:

  1. When opening an issue, attach a minimal .pgaf file describing just the topology and the manual steps that trigger the problem (a cluster{} block, a setup{} that gets to the interesting state, and network/sql/exec commands reproducing the failure — no step{}/sequence needed yet). pgaftest show compose <spec.pgaf> lets the reporter sanity-check the generated Docker Compose file without needing to actually understand pgaftest's internals.
  2. When triaging, a maintainer runs pgaftest tmux <attached-spec.pgaf> and gets the exact reported topology live in under a minute, with pg_autoctl watch showing FSM state in real time and docker compose logs -f showing every node's logs — no need to describe the bug in prose or guess at timing from a text report, and no need to reconstruct the environment from scratch.
  3. While diagnosing, pgaftest sql <node> { ... }, pgaftest network disconnect/connect <node>, and pgaftest show state let the maintainer poke at the live cluster interactively — inject the same fault differently, check intermediate state, confirm a hypothesis — without editing the spec or restarting the stack.
  4. Once the root cause is understood and fixed, the reporter's manual steps get turned into named step{}s with wait until/assert/expect assertions and a sequence (exactly what happened between the original issue-2 nodes + witness = 3 data centers (problem case detected) #997 repro and replication_stall_3dc.pgaf above), and pgaftest run <spec.pgaf> gives the same scenario a permanent, deterministic, TAP-emitting place in CI — so the exact bug that was reported can never silently come back.

The whole loop — reporter's repro spec straight through to CI regression test — uses the same file and the same tool the whole way, which is the main advantage over a prose bug report: nobody has to translate "here's what I did" into "here's a test" by hand.

Rebase notes

Rebased onto current main (post-#1150) on 2026-07-22; the earlier squashed pgaftest-development commits this branch used to carry (CLI restructure, pg_autoctl watch column-count fix, installcheck expected-output fixes, etc.) are already on main via #1150 and are no longer part of this PR's diff. Two pre-existing bugs were fixed while rebasing: a stray trailing backslash in the Dockerfile that broke every Docker build, and an incomplete 2.2→2.3 version bump (PG_AUTOCTL_EXTENSION_VERSION in defaults.h was left at "2.2", which made every pg_autoctl start try to downgrade the extension). tests/tap/specs/upgrade.pgaf's hardcoded version strings were also updated to match (its PREV_TAG auto-detection now resolves to v2.2 instead of v2.1, since it always targets the newest released tag below the current in-development version).

@dimitri dimitri self-assigned this Jul 15, 2026
@dimitri dimitri added the bug Something isn't working label Jul 15, 2026
@dimitri
dimitri force-pushed the fix/issue-997-replication-stall branch from 243af2b to 3218785 Compare July 21, 2026 23:35
dimitri added a commit that referenced this pull request Jul 22, 2026
pg_regress runs an unscheduled (bare) REGRESS list as a single parallel
group. None of src/monitor/'s regression tests were written to tolerate
that: they all register nodes via pgautofailover.register_node() against
the one pgautofailover.node table (and its one shared nodeid sequence) in
the whole contrib_regression database, and their expected/*.out files
hardcode small literal node ids on the assumption that they have that
sequence -- and the table -- to themselves. dummy_update.sql and
upgrade.sql additionally run their own CREATE/ALTER/DROP EXTENSION
pgautofailover against the single shared extension object, a second,
separate way for two tests to corrupt each other if run concurrently.

Observed in CI as sporadic "not ok - upgrade" and
"not ok - lock_and_fetch_migration" failures (most recently on PR #1149's
"Build run image (PG18)" job), depending on which pair of tests the
scheduler happened to interleave that run. Confirmed locally: running the
old flat REGRESS list surfaced a *second*, previously-undiagnosed instance
of the same class of bug -- monitor/workers/etc. leaking node-id and
formation rows into each other when run in a naive "everything but the
three extension-owning tests" parallel group.

Fix: add src/monitor/regress_schedule, a real pg_regress --schedule file
that puts every test in the original REGRESS list on its own serial
"test:" line, in dependency order (create_extension first, then the
functional tests, then dummy_update -> drop_extension -> upgrade last).
Wire it in via REGRESS_OPTS += --schedule=..., and override installcheck
to invoke pg_regress without also passing the bare $(REGRESS) list --
pg_regress treats trailing bare test-name arguments as extra tests run
one at a time *after* the schedule finishes (pg_regress.c's
extra_tests/run_single_test()), so passing both would silently re-run
every test a second time.

REGRESS itself is left in place, still listing every test: PGXS's
`ifdef REGRESS` gates the installcheck/clean recipes entirely (verified:
an empty-valued REGRESS makes `ifdef REGRESS` false), and it still
documents the full test set.

Verified: `make build-pg18` (the exact Dockerfile/pg_virtualenv path CI
uses) -- all 12 REGRESS tests and all 6 ISOLATION tests pass, in the
expected serialized order.
dimitri added 3 commits July 22, 2026 13:32
When the primary and standby lose connectivity while both remain reachable
from the monitor (the 3-DC split-brain scenario), synchronous_standby_names
stays set on the primary, causing every COMMIT to hang indefinitely.

Fix
---
Monitor-side FSM rule: when a PRIMARY node reports an empty
pg_stat_replication sync_state for longer than
pgautofailover.replication_stall_timeout (default 10 s), the monitor
assigns wait_primary.  This clears synchronous_standby_names and unblocks
writes without initiating a failover; the standby rejoins as secondary once
the link is restored.

Implementation details
----------------------
* pgautofailover.node gains two new columns:
    region                  text NOT NULL DEFAULT 'default'
    replication_stall_since timestamptz   (NULL = not stalled)

* Extension version bumped 2.2 → 2.3; upgrade script
  pgautofailover--2.2--2.3.sql added.

* register_node() gains a node_region parameter (default 'default').

* current_state() now returns noderegion so pg_autoctl watch can display
  the region column in verbose+ policies.

* ReportAutoFailoverNodeState() manages replication_stall_since: it is set
  to COALESCE(replication_stall_since, now()) when the node is in PRIMARY
  state and reportedrepstate is empty, and cleared (set to NULL) otherwise.

* New GUC: pgautofailover.replication_stall_timeout (10 s, PGC_SIGHUP).

* FSM rule in ProceedGroupStateFromContext(): if the primary is healthy
  (monitor can reach it) but replication_stall_since is set and
  now - replication_stall_since > replication_stall_timeout, assign
  wait_primary with a diagnostic log message.

* pg_autoctl watch: added COLUMN_TYPE_REGION, shown in verbose/almost-full/
  full/fully-verbose policies.

Test
----
tests/tap/specs/replication_stall_3dc.pgaf — a new regression spec that:
  1. Confirms sync replication is active.
  2. Severs the node1↔node2 Docker network link (simulating DC1↔DC2 failure).
  3. Waits for the primary to reach wait_primary (stall timeout = 3 s here).
  4. Verifies writes no longer hang (synchronous_standby_names is empty).
  5. Restores the link and verifies the standby rejoins and replication is
     healthy again.

Closes #997
- Thread --region option (-G) through all three pg_autoctl create postgres
  long_options arrays in cli_create_node.c, central option parsing in
  cli_common.c (case 'G'), keeper_config KeeperConfig.region field,
  nodespec.c ini/argv builder, and monitor_register_node() SQL call ().

- Bump extension version to 2.3 consistently: Makefile EXTVERSION, metadata.h
  AUTO_FAILOVER_EXTENSION_VERSION, and pgautofailover.control already at 2.3.
  Add pgautofailover--2.3--dummy.sql upgrade path for dummy_update regress test.
  Update dummy_update.out expected DETAIL line to say '2.3'.

- Fix monitor regress test ordering: add ORDER BY nodeid to the unordered
  SELECT from pgautofailover.node so the result is deterministic regardless
  of heap page layout (wider rows from new region/replication_stall_since
  columns changed physical scan order).

- Update watch_colspecs.h MAX_COL_SPECS 12→14 to accommodate the new
  COLUMN_TYPE_REGION entry plus COLUMN_TYPE_LAST sentinel in the fully-verbose
  column policy (13 entries total).

- Add pgaftest DSL support for 'region <name>' node option: T_REGION token in
  lexer, grammar rule in parser, TestNode.region field, compose_gen writes
  --region flag, cli_indent pretty-prints it.

- Add tests/tap/specs/replication_stall_3dc.pgaf: 2-node cluster across
  dc1/dc2 regions; cuts network to node2, waits for node1 to reach
  wait_primary (FSM rule fires after replication_stall_timeout), then
  restores connectivity and waits for secondary recovery.
Rebasing issue #997's replication-stall fix onto current main (which
bumps the extension to 2.3) shifts tests/upgrade/Makefile's PREV_TAG
auto-detection: it excludes whatever tag matches the *current*
in-development version, so with current=2.3 it now resolves to v2.2 (the
latest actual release) instead of v2.1. upgrade.pgaf hardcoded '2.1'/'2.2'
version strings that no longer match:

  - baseline `installed_version` check: 2.1 -> 2.2
  - `ALTER EXTENSION ... UPDATE TO`: '2.2' -> '2.3'
  - final `installed_version` check: 2.2 -> 2.3

Also updated the header comment: the documented "v2.1 supervisor bug"
(no monitor_init before the dropped-node check, fixed in v2.2) no longer
applies to a v2.2 -> v2.3 upgrade, so the "compose start" calls in
test_003b_wait_keeper_restart are now expected no-ops against
still-running containers rather than a real crash recovery -- kept as-is
so the spec still exercises the restart path correctly whenever PREV_TAG
is pinned back to a pre-v2.2 release (PREV_TAG=v2.1 make ...).

Verified: `pgaftest run tests/tap/specs/upgrade.pgaf` against PG16,
10/10 steps passing, PREV_TAG correctly auto-detected as v2.2.
@dimitri
dimitri force-pushed the fix/issue-997-replication-stall branch from 3218785 to 5294ec5 Compare July 22, 2026 11:33
pg_regress's "upgrade" test tries expected/upgrade.out first, then falls
back to expected/upgrade_1.out (the variant matching psql versions that
show a "Default version" column in \dx output) -- CI runners hit the
second one. That file still hardcoded "2.2" as the control file's
default_version, left over from before this branch bumped the extension
to 2.3; the 2.2 -> 2.3 bump touched pgautofailover.control and
defaults.h but missed this expected-output file, so every \dx assertion
after CREATE/ALTER EXTENSION (1.0, 1.1, 1.2, 1.3) failed on the
"Default version" column.

src/monitor/expected/pg19/expected/upgrade_1.out is a symlink to this
same file, so no separate edit needed there.

Verified: `make build-pg18` (the exact Dockerfile/pg_virtualenv path CI
uses) -- all 12 REGRESS tests pass, including upgrade.
dimitri added 2 commits July 22, 2026 15:07
6a8be97 ("Wire --region through CLI, fix extension versioning, add 3-DC
stall test") added "order by nodeid" to the unordered SELECT from
pgautofailover.node in sql/monitor.sql, to make the result deterministic
now that wider rows (new region/replication_stall_since columns) can
change physical heap scan order. It updated the base
expected/monitor.out to match, but missed updating the PG19-specific
copy at expected/pg19/expected/monitor.out (kept as a real file, not a
symlink, because of PG19's differing pg_lsn display format elsewhere in
the same test) -- so the "monitor" test failed under PG19 CI with the
query missing its ORDER BY clause.

Verified: `make build-pg19` -- clean build, all 12 REGRESS tests pass.
6a8be97 ("Wire --region through CLI, fix extension versioning, add 3-DC
stall test") added a noderegion column to the monitor_get_current_state
query at index 14, shifting healthlag/reportlag to indices 15/16 (17
columns total, 0-16) -- and updated the per-row parser and its own
documentation comment to match. It missed the PQnfields() guard a few
lines above, which still required exactly 16 columns, so every call
through this path failed immediately with "Query returned 17 columns,
expected 16" before the (already-correct) row parsing ever ran.

This is the single root cause behind widespread CI failures across
every PG version: pytest single/ssl (pg_autoctl show state), pgaftest
quick's config_get_set spec (same path, via `pg_autoctl set node
metadata` + `show state`), and both citus-1/citus-2 specs (coordinator
fetching the node list from the monitor during formation setup).

Verified: rebuilt the run + pgaftest images and re-ran the exact
previously-failing `quick` schedule against them -- config_get_set now
passes cleanly, no more column-count error anywhere in the run.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 nodes + witness = 3 data centers (problem case detected)

1 participant